Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] display preview warning and decorator info #26819

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Jan 3, 2025

Summary & Motivation

Same as #25362 but for the new preview decorator introduced in #26747

Copy link

github-actions bot commented Jan 3, 2025

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-l58pe1ro9-elementl.vercel.app
https://maxime-ad-758-add-preview-docs.dagster.dagster-docs.io

Direct link to changed pages:

@@ -104,6 +104,10 @@ dt > a.reference.internal {
@apply inline-flex items-center px-3 py-0.5 rounded-full align-middle text-xs uppercase font-medium bg-sea-foam text-gable-green
}

.preview-tag {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need a .preview-tag.flag.preview for the API docs? Looking at superseded and deprecated, we have two different CSS per flag, but it's not the case for experimental.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That CSS hasn't been touched in quite some time, but it looks like @hellendag was the one to do it. Perhaps he would know. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the [foo]-tag classname is added in MDX components, and flag [foo] is added via Sphinx, and I think to be safest we would want to include both selectors.

.preview-tag,
.flag.preview {
  ...
}

So any of the rules where they are currently separate (e.g. .superseded-tag has a separate rule from .flag.superseded), we probably want them to be combined instead.

Additionally:

  • There are two rules below that have .legacy-tag, but only one should. (I think probably not the flag.superseded one?)
  • .deprecated-tag and .flag.deprecated have different styles, which seems like it probably wasn't intentional. Maybe these should be unified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hellendag ! I updated the CSS for the preview flag c70913b and fixed the superseded and deprecated ones here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS here and in the other PR lgtm, thanks!

@@ -104,6 +104,11 @@ dt > a.reference.internal {
@apply inline-flex items-center px-3 py-0.5 rounded-full align-middle text-xs uppercase font-medium bg-sea-foam text-gable-green
}

.preview-tag,
.flag.preview{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter should probably catch this, but

Suggested change
.flag.preview{
.flag.preview {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!

@maximearmstrong maximearmstrong merged commit 18fbec5 into master Jan 9, 2025
3 checks passed
@maximearmstrong maximearmstrong deleted the maxime/ad-758/add-preview-docs branch January 9, 2025 16:31
maximearmstrong added a commit that referenced this pull request Jan 9, 2025
## Summary & Motivation

Updates the CSS for superseded and deprecated flags as discussed
[here](#26819 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants